Adds screenshot options to context menu and commands #27083

jonathansampson · 2024-12-21T12:55:50Z

Resolves brave/brave-browser#25816
Security/Privacy Review: https://github.com/brave/reviews/issues/1831

screenshot-context-menu.mp4

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Right-click on a page
Click Screenshot Tools from the context menu
Select an option (i.e., Selection, Viewport, Full page) from the submenu

Once a screenshot has been taken, you should see a UI element (with a preview of your screenshot) informing you that the screenshot has been copied to your device's clipboard.

jonathansampson · 2025-01-02T15:03:09Z

I've refactored this into a TabHelper pattern, with deferred attachment (until user requests a screenshot). Commits landing here in a bit.

bridiver · 2025-01-02T18:39:33Z

I've refactored this into a TabHelper pattern, with deferred attachment (until user requests a screenshot). Commits landing here in a bit.

Make sure you follow the latest updates using TabInterface and not WebContentsUserData

jonathansampson · 2025-01-02T18:40:42Z

I've refactored this into a TabHelper pattern, with deferred attachment (until user requests a screenshot). Commits landing here in a bit.

Make sure you follow the latest updates using TabInterface and not WebContentsUserData

Looking into that now. Thanks!

app/brave_generated_resources.grd

browser/brave_screenshots/screenshots_tab_feature.h

browser/brave_screenshots/screenshots_utils.cc

chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc

fallaciousreasoning

lgtm % nits

By using a browser reference we can avoid the reliance on global methods to look up a window from a given webcontents instance.

Removed the helper "TakeScreenshot" method which originally existed to stand up and instance of the helper. Now that the helper is a proper tab feature, we can use it as such from context menu commands and elsewhere.

If there is nothing selected, we shouldn't attempt to construct an autocomplete match, run the autocomplete classifier, etc. Furthermore, this enables us to simplify tests involving the context menu due to reduced setup.

After rebasing, there is now a failure in this PR unrelated to the contents or changes introduced by the PR itself. A fresh build from scratch will resolve this, but a component build will proceed if any change is made to the policy template file. As such, a typo has been found and fixed so as to 1) Improve the comments, and importantly 2) Fix the build 😀 See https://bravesoftware.slack.com/archives/C7VLGSR55/p1736928018175489?thread_ts=1736911378.517619&cid=C7VLGSR55 for more info.

darkdh · 2025-01-17T18:36:41Z

browser/brave_screenshots/BUILD.gn

+  ]
+
+  deps = [
+    "//chrome/browser/image_editor:image_editor",


this can be just //chrome/browser/image_editor same as "//chrome/browser/ui and //ui/base/clipboard

I purposefully provide the target names, just to more clearly communicate intent. If it's okay, I'd prefer to keep them explicit.

browser/brave_screenshots/screenshots_tab_feature.h

browser/brave_screenshots/screenshots_tab_feature.cc

browser/brave_screenshots/strategies/fullpage_strategy.cc

browser/brave_screenshots/test/brave_screenshots_unittests.cc

github-actions · 2025-01-18T03:29:05Z

[puLL-Merge] - brave/brave-core@27083

Description

This PR introduces screenshot functionality to the Brave browser, allowing users to capture full-page, viewport, or selected area screenshots directly from the browser's context menu. The feature is gated behind a feature flag and includes new command IDs, UI strings, and implementation files for different screenshot strategies.

Changes

Changes

app/brave_command_ids.h:
- Added new command IDs for screenshot tools and specific screenshot actions.
app/brave_generated_resources.grd and app/brave_screenshots_strings.grdp:
- Added new string resources for screenshot menu items and options.
browser/about_flags.cc:
- Added a new flag "brave-screenshots" to enable/disable the screenshot feature.
browser/brave_screenshots/:
- Added new files for screenshot feature implementation, including strategies for full-page, viewport, and selection screenshots.
browser/ui/brave_browser_command_controller.cc:
- Updated to handle new screenshot commands.
browser/ui/browser_commands.cc and browser/ui/browser_commands.h:
- Added a new function TakeScreenshot to handle screenshot actions.
chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc:
- Modified to include the new screenshot menu items in the context menu.
chromium_src/chrome/browser/ui/tabs/tab_features.cc:
- Added initialization for the Brave screenshots feature.

sequenceDiagram
    participant User
    participant ContextMenu
    participant BraveScreenshotsTabFeature
    participant ScreenshotStrategy
    participant DevTools
    participant Clipboard

    User->>ContextMenu: Right-click and select screenshot option
    ContextMenu->>BraveScreenshotsTabFeature: StartScreenshot
    BraveScreenshotsTabFeature->>ScreenshotStrategy: Create strategy
    ScreenshotStrategy->>DevTools: Capture screenshot (if full-page)
    DevTools-->>ScreenshotStrategy: Return screenshot data
    ScreenshotStrategy-->>BraveScreenshotsTabFeature: Return capture result
    BraveScreenshotsTabFeature->>Clipboard: Copy image to clipboard
    BraveScreenshotsTabFeature->>User: Display screenshot bubble

Possible Issues

The full-page screenshot strategy uses DevTools protocol, which might have performance implications for very large pages.
The feature is currently limited to desktop platforms (Windows, macOS, Linux).

Security Hotspots

browser/brave_screenshots/strategies/fullpage_strategy.cc:
- Uses DevTools protocol to capture full-page screenshots. Ensure proper sanitization of input and output to prevent potential security vulnerabilities.
browser/brave_screenshots/screenshots_utils.cc:
- Handles copying images to the clipboard. Ensure proper handling of large images to prevent potential memory issues or crashes.

jonathansampson added enhancement CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Dec 21, 2024

jonathansampson self-assigned this Dec 21, 2024

github-actions bot added the CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) label Dec 21, 2024

jonathansampson marked this pull request as ready for review December 23, 2024 04:48

jonathansampson requested review from a team and bridiver as code owners December 23, 2024 04:48

jonathansampson marked this pull request as draft December 26, 2024 22:48

jonathansampson force-pushed the sampson-brave-screenshots branch 2 times, most recently from 0e69a8a to 4b7451d Compare December 28, 2024 23:18

jonathansampson marked this pull request as ready for review December 29, 2024 02:58

jonathansampson requested review from fallaciousreasoning and cdesouza-chromium December 29, 2024 02:58

jonathansampson marked this pull request as draft January 2, 2025 21:17

jonathansampson force-pushed the sampson-brave-screenshots branch 2 times, most recently from 22868e1 to fdd6104 Compare January 4, 2025 17:34

github-actions bot added the CI/run-network-audit Run network-audit label Jan 4, 2025

fallaciousreasoning reviewed Jan 5, 2025

View reviewed changes

app/brave_generated_resources.grd Outdated Show resolved Hide resolved

fallaciousreasoning reviewed Jan 5, 2025

View reviewed changes

browser/brave_screenshots/screenshots_tab_feature.h Outdated Show resolved Hide resolved

fallaciousreasoning reviewed Jan 5, 2025

View reviewed changes

browser/brave_screenshots/screenshots_utils.cc Outdated Show resolved Hide resolved

fallaciousreasoning reviewed Jan 5, 2025

View reviewed changes

chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc Outdated Show resolved Hide resolved

fallaciousreasoning approved these changes Jan 5, 2025

View reviewed changes

jonathansampson marked this pull request as ready for review January 7, 2025 14:04

jonathansampson added 21 commits January 15, 2025 09:17

refactors to use browser reference

2198da7

By using a browser reference we can avoid the reliance on global methods to look up a window from a given webcontents instance.

more accurate feature description

5b5682c

various simplifications

8197ef8

Removed the helper "TakeScreenshot" method which originally existed to stand up and instance of the helper. Now that the helper is a proper tab feature, we can use it as such from context menu commands and elsewhere.

moves screenshot strings to own file

1b71d0e

Don't show the submenu within devtools

6bf8a54

refactors devtools code into distinct helper

50798f7

pesubmit fixes

0f78167

avoid forward declarations per style guide

c86dabb

Skip autocomplete classifier on empty selection

a2887ec

If there is nothing selected, we shouldn't attempt to construct an autocomplete match, run the autocomplete classifier, etc. Furthermore, this enables us to simplify tests involving the context menu due to reduced setup.

adds context-menu related unittests for screenshots

92f82e3

cleanup and simplify unit tests

317462b

dependency cleanup

748c3c6

moves to strategy-based screenshotting

fd5d3c3

Simplifies clip-indicating member

bba8861

Update screenshots_tab_feature.cc

f76198e

adds some comments around screenshot limitations

32d4f72

minor note changes

194be01

removes unused includes

ee4b82e

clean up build.gn deps

9ebdb08

uses explicit/intentional targets

01c9de6

uses debug logs

6d39b1e

jonathansampson force-pushed the sampson-brave-screenshots branch from f8c8d02 to 6d39b1e Compare January 15, 2025 17:02

jonathansampson requested a review from darkdh January 16, 2025 13:09

darkdh reviewed Jan 17, 2025

View reviewed changes

addressing minor feedback items

3f4ba33

jonathansampson mentioned this pull request Jan 18, 2025

[Screenshots] Notify when screenshot cropped brave/brave-browser#43369

Open

adds GitHub Issue reference

aecc9d0

jonathansampson requested a review from darkdh January 18, 2025 03:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds screenshot options to context menu and commands #27083

Adds screenshot options to context menu and commands #27083

jonathansampson commented Dec 21, 2024 •

edited

Loading

jonathansampson commented Jan 2, 2025

bridiver commented Jan 2, 2025

jonathansampson commented Jan 2, 2025

fallaciousreasoning left a comment

darkdh Jan 17, 2025

jonathansampson Jan 18, 2025

github-actions bot commented Jan 18, 2025

Changes

Adds screenshot options to context menu and commands #27083

Are you sure you want to change the base?

Adds screenshot options to context menu and commands #27083

Conversation

jonathansampson commented Dec 21, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

jonathansampson commented Jan 2, 2025

bridiver commented Jan 2, 2025

jonathansampson commented Jan 2, 2025

fallaciousreasoning left a comment

Choose a reason for hiding this comment

darkdh Jan 17, 2025

Choose a reason for hiding this comment

jonathansampson Jan 18, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2025

Description

Changes

Possible Issues

Security Hotspots

jonathansampson commented Dec 21, 2024 •

edited

Loading